bpo-37953:Improve ForwardRef equality checks#15628
Conversation
brandtbucher
left a comment
brandtbucher
left a comment
There was a problem hiding this comment.
This looks like a good change. I found one optimization that can be made, below.
Can you also add regression tests as well?
| if p not in new_params: | ||
| new_params.append(p) | ||
| params = new_params | ||
| return tuple(params) |
There was a problem hiding this comment.
This whole bit can be sped up by just replacing it with:
return tuple(dict.fromkeys(params))There was a problem hiding this comment.
Thanks, I will add some tests. By the way, params is a list, tuple(dict.fromkeys(params)) seems not faster than tuple(params), I think it would be better to keep tuple(params):
There was a problem hiding this comment.
Sorry, I should have been clearer! I meant replacing this whole list + loop bit with that one line. dict.fromkeys is a nice shortcut to remove duplicates while preserving order.
There was a problem hiding this comment.
Ah... I see now. You're trying to avoid the (broken) hashing.
I don't know how I feel about that. I think hashing should be fixed rather than just sidestepped here, since it's so tied to equality. At this point I'm actually in favor of a hybrid approach: combining your fix for __eq__ and the other PR #15400’s fix for __hash__.
@gvanrossum and @ilevkivskyi, thoughts?
There was a problem hiding this comment.
@hongweipeng you can revert your most recent commit (3676a50, “code review”). My comment was misguided!
| params = new_params | ||
| assert not all_params, all_params | ||
| return tuple(params) | ||
| return tuple(dict.fromkeys(params)) |
There was a problem hiding this comment.
TBH this looks like a hack. If at some point we would make sets ordered the same way as dicts are, it would be good to update the code, but currently I think I like the old way more.
|
See also my comment #15400 (comment) in the other PR. |
|
PR #15650 is better. |
5 participants
https://bugs.python.org/issue37953